Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Helm - custom service account creation and management #17880

Merged

Conversation

wiktor2200
Copy link
Contributor

@wiktor2200 wiktor2200 commented Dec 28, 2021

SUMMARY

Possibility to create custom service account in helm chart.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

I've tested all possible changes on parameters:

  • serviceAccountName
  • serviceAccount.create
    All conditionals in _helpers file works as expected.

ADDITIONAL INFORMATION

  • Has associated issue: Helm chart configure and create custom service account #17879
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@amitmiran137
Copy link
Member

Could you bump version of the chart manually ?

@wiktor2200
Copy link
Contributor Author

Sure! It's done :)

@wiktor2200 wiktor2200 changed the title feat: Custom service account creation and management feat: Helm - custom service account creation and management Dec 29, 2021
@wiktor2200
Copy link
Contributor Author

@amitmiran137 there is something wrong with workflow. It shows error in code which I didn't change.
BTW, who can be assigned as reviewer for this PR? Are you maintainer of helm charts?

Copy link

@przemyslawandruszewski przemyslawandruszewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you please also put the serviceAccountName to "init-job" also?

# Create custom service account for Superset. If create: true and name is not provided, superset.fullname will be used.
# serviceAccountName: superset
serviceAccount:
create: false

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

  1. Remove "serviceAccountName"
  2. creating serviceAccount sestion as you did with two properties:
  • create as you did
  • name

According to: https://helm.sh/docs/chart_best_practices/rbac/#yaml-configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought about it. That was even my first choice, but then I've noticed that, there was ServiceAccountName variable added in the past #15340, so I've implemented new feature as enhancement beside the current solution.
I don't want to destroy any configs that can exist somewhere.
@mvoitko and @craig-rueda can you give some more feedback about this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeps you are right, we would break backward compatibility but on the other site, we could maintain both for sometime and prepare a communication about that right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone have blocked version on helm in their definition it's OK, but when someone is using "latest" it will brake down deployment (or dependent components).
Supporting two solutions for a while should be quite easy (I'll just add one more conditions in _helpers) but this could cause some caveats such as overwriting values if serviceAccountName and serviceAccount.name would be defined in the same time. 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it LGTM. If people are using "latest" in any env that matters, that's generally a bad practice. Using the default of create: false seems reasonable to me.

@wiktor2200
Copy link
Contributor Author

@przemyslawandruszewski I've added support for custom service account in init-job in this commit: 372fed1

@amitmiran137
Copy link
Member

@amitmiran137 there is something wrong with workflow. It shows error in code which I didn't change.
BTW, who can be assigned as reviewer for this PR? Are you maintainer of helm charts?

Yes we became aware lately
It seems to be broken also in master
We are trying to solve this

@przemyslawandruszewski
Copy link

@wiktor2200 I cannot find any template for creating service account when serviceAccount.create == true. Could you validate whether you commited all the stuff?

@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 29, 2021
@wiktor2200
Copy link
Contributor Author

@przemyslawandruszewski you got right, thanks for notifying that.
IDK how this happened, but I've not commited this file. Maybe I run git add with some regexp and it has not been added 🤷‍♂️

@przemyslawandruszewski
Copy link

The rest is fine for me, let's leave final decision for @craig-rueda :)

@wiktor2200
Copy link
Contributor Author

@craig-rueda Pipeline failed because of missing license header. I've added it, could you re-approve? :)

@przemyslawandruszewski
Copy link

@wiktor2200 looks like you can merge :)

@wiktor2200
Copy link
Contributor Author

Unfortunately I can't. :( It has to be clicked by someone with write access to repo.
screen

@przemyslawandruszewski
Copy link

Unfortunately I can't. :( It has to be clicked by someone with write access to repo. screen

Sorry then :) So the request goes to @craig-rueda :)

@villebro villebro merged commit 6991417 into apache:master Jan 4, 2022
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
…7880)

* feat: Custom service account creation and management

* bump helm chart version

* add custom service account in init-job

* service account creation template

* changed service account creation template

* add license
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
…7880)

* feat: Custom service account creation and management

* bump helm chart version

* add custom service account in init-job

* service account creation template

* changed service account creation template

* add license
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants